Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ms2/layoutmenu: show selected item #310

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

FelipeTrost
Copy link
Contributor

Summary

show selected item in layout menu

This comment has been minimized.

Comment on lines +62 to +77
let activeKey: string | undefined;
for (const layoutItem of layoutMenuItems) {
if (
layoutItem === null ||
!('type' in layoutItem) ||
layoutItem.type !== 'group' ||
!layoutItem.children
)
continue;

activeKey = layoutItem?.children.find((item) => {
return item?.key && pathname.includes(item.key as string);
})?.key as string;

if (activeKey) break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could fail if we have a menu item like /executions as well as /executions/monitoring or sth. like that. If you want to do it that way, the check should be 1:1. Alternatively a context could be set through a Content component prop that gets read here, so we aren't limited by the url and still set it through the individual pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think the best way is through some form of url mathing, otherwise it would become to annoying to have to set a context value every time, and also there could be some issues with intercepting routes. What do you think of checking that the url ends in a string? This could also cause conflicts, but it is far less likely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain that more, what do you mean with ends with a string? How would you make sure /executions/list or whatever could be at the end marks the /executions list item in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the url is ".../executions/list", then url.endsWith("/executions") would be false, and url.endsWith("/executions/list") would be true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I misunderstood then maybe. I though the intention was to always mark an item, even on sub-sites (like only executions is in the menu, so /executions/list would still mark executions item in menu).

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented Jul 7, 2024

CLOUDRUN ACTIONS

✅ Successfully created Preview Deployment.

https://pr-310---ms-server-staging-c4f6qdpj7q-ew.a.run.app

@FelipeTrost FelipeTrost marked this pull request as draft July 25, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants